Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: append h2 to tlsconfig.NextProtos #2744

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Conversation

menghanl
Copy link
Contributor

@menghanl menghanl commented Apr 4, 2019

fix #2729

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any interaction between this PR and #2742?

credentials/credentials.go Show resolved Hide resolved
return ps
}
}
ret := make([]string, 0, len(ps)+1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an unnecessary optimisation since the underlying array would at most end up as 4-10 elements in usual cases if the capacity was spent before an append. And this only happens when creating a new transport credential. Out of curiosity: is there something I'm missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't modify the underlying array, as it's owned by the user, so it needs to be copied first. Since we know the final size of it, there's no reason not to allocate the right amount of memory up front.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation dfawley, that makes a lot of sense!

@kristoiv
Copy link

kristoiv commented Apr 5, 2019

I can confirm that this solves my problem in issue #2729 . It neatly block requests from a grpc client while it creates the certificate in the background and as soon as it is ready it completes the request. This works flawlessly with only port 443 opened.

Hope this doesn't affect anything else and that we can get it into production soon :)

@menghanl menghanl merged commit 4abb362 into grpc:master Apr 8, 2019
@menghanl menghanl deleted the alpn branch April 8, 2019 16:56
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support using letsencrypt with tls-alpn-01
3 participants